From 1a7a48844c783c9c517ac57ba48f90159c01c3f8 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 24 Jun 2014 22:02:21 -0700 Subject: [PATCH] Move from all-URL to Path/URL enum On windows a path cannot be represented as a file:// URL because of the backslashes and colons in the file name. This causes all of the tests which rely on git to fail on windows. This commit changes the representation of the location of a package to be an enum, Location, with two variants: Remote and Local. When parsing Cargo.toml, all locations which begin with the string "file:" have that prefix stripped and are then interpreted as Local packages. Everything else is parsed as a URL and used as a Remote package. --- src/cargo/core/package.rs | 2 +- src/cargo/core/package_id.rs | 16 +++---- src/cargo/core/resolver.rs | 43 +++++++------------ src/cargo/core/source.rs | 76 +++++++++++++++++++++------------ src/cargo/sources/git/source.rs | 45 +++++++++++-------- src/cargo/sources/git/utils.rs | 25 +++++------ src/cargo/util/toml.rs | 26 +++++++---- 7 files changed, 129 insertions(+), 104 deletions(-) diff --git a/src/cargo/core/package.rs b/src/cargo/core/package.rs index 1bf19fab6..32284f82f 100644 --- a/src/cargo/core/package.rs +++ b/src/cargo/core/package.rs @@ -123,7 +123,7 @@ impl Package { // Sort the sources just to make sure we have a consistent fingerprint. sources.sort_by(|a, b| { cmp::lexical_ordering(a.kind.cmp(&b.kind), - a.url.to_str().cmp(&b.url.to_str())) + a.location.to_str().cmp(&b.location.to_str())) }); let sources = sources.iter().map(|source_id| { source_id.load(config) diff --git a/src/cargo/core/package_id.rs b/src/cargo/core/package_id.rs index d96e21547..f777c0756 100644 --- a/src/cargo/core/package_id.rs +++ b/src/cargo/core/package_id.rs @@ -10,7 +10,8 @@ use serialize::{ Decoder }; -use util::{CargoResult, CargoError}; +use util::{CargoResult, CargoError, FromError}; +use core::source::Location; trait ToVersion { fn to_version(self) -> Result; @@ -57,7 +58,7 @@ impl<'a> ToUrl for &'a Url { pub struct PackageId { name: String, version: semver::Version, - namespace: Url + namespace: Location, } #[deriving(Clone, Show, PartialEq)] @@ -77,14 +78,13 @@ impl CargoError for PackageIdError { } impl PackageId { - pub fn new(name: &str, version: T, - namespace: U) -> CargoResult { + pub fn new(name: &str, version: T, + ns: &Location) -> CargoResult { let v = try!(version.to_version().map_err(InvalidVersion)); - let ns = try!(namespace.to_url().map_err(InvalidNamespace)); Ok(PackageId { name: name.to_str(), version: v, - namespace: ns + namespace: ns.clone() }) } @@ -96,7 +96,7 @@ impl PackageId { &self.version } - pub fn get_namespace<'a>(&'a self) -> &'a Url { + pub fn get_namespace<'a>(&'a self) -> &'a Location { &self.namespace } } @@ -125,7 +125,7 @@ impl>> Decodable> for PackageId { PackageId::new( vector.get(0).as_slice(), vector.get(1).as_slice(), - vector.get(2).as_slice()) + &Location::parse(vector.get(2).as_slice()).unwrap()) } } diff --git a/src/cargo/core/resolver.rs b/src/cargo/core/resolver.rs index da0dcb834..dc81e5cb8 100644 --- a/src/cargo/core/resolver.rs +++ b/src/cargo/core/resolver.rs @@ -57,56 +57,43 @@ pub fn resolve(deps: &[Dependency], #[cfg(test)] mod test { use url; + use hamcrest::{assert_that, equal_to, contains}; - use hamcrest::{ - assert_that, - equal_to, - contains - }; - - use core::source::{ - SourceId, - RegistryKind - }; - - use core::{ - Dependency, - PackageId, - Summary - }; - - use super::{ - resolve - }; + use core::source::{SourceId, RegistryKind, Location, Remote}; + use core::{Dependency, PackageId, Summary}; + use super::resolve; macro_rules! pkg( ($name:expr => $($deps:expr),+) => ( { let url = url::from_str("http://example.com").unwrap(); - let source_id = SourceId::new(RegistryKind, url); + let source_id = SourceId::new(RegistryKind, Remote(url)); let d: Vec = vec!($($deps),+).iter().map(|s| { Dependency::parse(*s, Some("1.0.0"), &source_id).unwrap() }).collect(); - Summary::new(&PackageId::new($name, "1.0.0", - "http://www.example.com/").unwrap(), + Summary::new(&PackageId::new($name, "1.0.0", ®istry_loc()).unwrap(), d.as_slice()) } ); ($name:expr) => ( - Summary::new(&PackageId::new($name, "1.0.0", - "http://www.example.com/").unwrap(), []) + Summary::new(&PackageId::new($name, "1.0.0", ®istry_loc()).unwrap(), + []) ) ) + fn registry_loc() -> Location { + Location::parse("http://www.example.com/").unwrap() + } + fn pkg(name: &str) -> Summary { - Summary::new(&PackageId::new(name, "1.0.0", "http://www.example.com/").unwrap(), + Summary::new(&PackageId::new(name, "1.0.0", ®istry_loc()).unwrap(), &[]) } fn dep(name: &str) -> Dependency { let url = url::from_str("http://example.com").unwrap(); - let source_id = SourceId::new(RegistryKind, url); + let source_id = SourceId::new(RegistryKind, Remote(url)); Dependency::parse(name, Some("1.0.0"), &source_id).unwrap() } @@ -116,7 +103,7 @@ mod test { fn names(names: &[&'static str]) -> Vec { names.iter() - .map(|name| PackageId::new(*name, "1.0.0", "http://www.example.com/").unwrap()) + .map(|name| PackageId::new(*name, "1.0.0", ®istry_loc()).unwrap()) .collect() } diff --git a/src/cargo/core/source.rs b/src/cargo/core/source.rs index 8636acf30..3214a8eb0 100644 --- a/src/cargo/core/source.rs +++ b/src/cargo/core/source.rs @@ -4,9 +4,10 @@ use std::fmt::{Show, Formatter}; use url; use url::Url; -use core::{Summary,Package,PackageId}; -use sources::{PathSource,GitSource}; -use util::{Config,CargoResult}; +use core::{Summary, Package, PackageId}; +use sources::{PathSource, GitSource}; +use util::{Config, CargoResult}; +use util::errors::human; /// A Source finds and downloads remote packages based on names and /// versions. @@ -51,20 +52,47 @@ pub enum SourceKind { RegistryKind } +#[deriving(Clone, PartialEq, Eq)] +pub enum Location { + Local(Path), + Remote(Url), +} + #[deriving(Clone,PartialEq)] pub struct SourceId { pub kind: SourceKind, - pub url: Url + pub location: Location, +} + +impl Show for Location { + fn fmt(&self, f: &mut Formatter) -> fmt::Result { + match *self { + Local(ref p) => write!(f, "file:{}", p.display()), + Remote(ref u) => write!(f, "{}", u), + } + } +} + +impl Location { + pub fn parse(s: &str) -> CargoResult { + if s.starts_with("file:") { + Ok(Local(Path::new(s.slice_from(5)))) + } else { + url::from_str(s).map(Remote).map_err(|e| { + human(format!("invalid url `{}`: `{}", s, e)) + }) + } + } } impl Show for SourceId { fn fmt(&self, f: &mut Formatter) -> fmt::Result { match *self { - SourceId { kind: PathKind, ref url } => { - try!(write!(f, "{}", url)) + SourceId { kind: PathKind, ref location } => { + try!(write!(f, "{}", location)) }, - SourceId { kind: GitKind(ref reference), ref url } => { - try!(write!(f, "{}", url)); + SourceId { kind: GitKind(ref reference), ref location } => { + try!(write!(f, "{}", location)); if reference.as_slice() != "master" { try!(write!(f, " (ref={})", reference)); } @@ -80,33 +108,26 @@ impl Show for SourceId { } impl SourceId { - pub fn new(kind: SourceKind, url: Url) -> SourceId { - SourceId { kind: kind, url: url } + pub fn new(kind: SourceKind, location: Location) -> SourceId { + SourceId { kind: kind, location: location } } // Pass absolute path pub fn for_path(path: &Path) -> SourceId { - // TODO: use proper path -> URL - let url = if cfg!(windows) { - let path = path.display().to_str(); - format!("file://{}", path.as_slice().replace("\\", "/")) - } else { - format!("file://{}", path.display()) - }; - SourceId::new(PathKind, url::from_str(url.as_slice()).unwrap()) + SourceId::new(PathKind, Local(path.clone())) } pub fn for_git(url: &Url, reference: &str) -> SourceId { - SourceId::new(GitKind(reference.to_str()), url.clone()) + SourceId::new(GitKind(reference.to_str()), Remote(url.clone())) } pub fn for_central() -> SourceId { SourceId::new(RegistryKind, - url::from_str("https://example.com").unwrap()) + Remote(url::from_str("https://example.com").unwrap())) } - pub fn get_url<'a>(&'a self) -> &'a Url { - &self.url + pub fn get_location<'a>(&'a self) -> &'a Location { + &self.location } pub fn is_path(&self) -> bool { @@ -124,12 +145,11 @@ impl SourceId { match self.kind { GitKind(..) => box GitSource::new(self, config) as Box, PathKind => { - let mut path = self.url.path.clone(); - if cfg!(windows) { - path = path.replace("/", "\\"); - } - let path = Path::new(path); - box PathSource::new(&path, self) as Box + let path = match self.location { + Local(ref p) => p, + Remote(..) => fail!("path sources cannot be remote"), + }; + box PathSource::new(path, self) as Box }, RegistryKind => unimplemented!() } diff --git a/src/cargo/sources/git/source.rs b/src/cargo/sources/git/source.rs index aa889f92f..1868023c0 100644 --- a/src/cargo/sources/git/source.rs +++ b/src/cargo/sources/git/source.rs @@ -1,13 +1,12 @@ +use std::fmt::{Show,Formatter}; use std::fmt; -use std::hash::sip::SipHasher; use std::hash::Hasher; -use std::fmt::{Show,Formatter}; +use std::hash::sip::SipHasher; use std::io::MemWriter; +use std::str; use serialize::hex::ToHex; -use url; -use url::Url; -use core::source::{Source,SourceId,GitKind}; +use core::source::{Source, SourceId, GitKind, Location, Remote, Local}; use core::{Package,PackageId,Summary}; use util::{CargoResult,Config}; use sources::PathSource; @@ -33,8 +32,8 @@ impl<'a, 'b> GitSource<'a, 'b> { _ => fail!("Not a git source; id={}", source_id) }; - let remote = GitRemote::new(source_id.get_url()); - let ident = ident(&source_id.url); + let remote = GitRemote::new(source_id.get_location()); + let ident = ident(source_id.get_location()); let db_path = config.git_db_path() .join(ident.as_slice()); @@ -54,23 +53,32 @@ impl<'a, 'b> GitSource<'a, 'b> { } } - pub fn get_namespace<'a>(&'a self) -> &'a url::Url { - self.remote.get_url() + pub fn get_namespace<'a>(&'a self) -> &'a Location { + self.remote.get_location() } } -fn ident(url: &Url) -> String { +fn ident(location: &Location) -> String { let hasher = SipHasher::new_with_keys(0,0); - let mut ident = url.path.as_slice().split('/').last().unwrap(); + // FIXME: this really should be able to not use to_str() everywhere, but the + // compiler seems to currently ask for static lifetimes spuriously. + // Perhaps related to rust-lang/rust#15144 + let ident = match *location { + Local(ref path) => { + let last = path.components().last().unwrap(); + str::from_utf8(last).unwrap().to_str() + } + Remote(ref url) => url.path.as_slice().split('/').last().unwrap().to_str() + }; - ident = if ident == "" { - "_empty" + let ident = if ident.as_slice() == "" { + "_empty".to_string() } else { ident }; - format!("{}-{}", ident, to_hex(hasher.hash(&url.to_str()))) + format!("{}-{}", ident, to_hex(hasher.hash(&location.to_str()))) } fn to_hex(num: u64) -> String { @@ -81,7 +89,7 @@ fn to_hex(num: u64) -> String { impl<'a, 'b> Show for GitSource<'a, 'b> { fn fmt(&self, f: &mut Formatter) -> fmt::Result { - try!(write!(f, "git repo at {}", self.remote.get_url())); + try!(write!(f, "git repo at {}", self.remote.get_location())); match self.reference { Master => Ok(()), @@ -98,7 +106,7 @@ impl<'a, 'b> Source for GitSource<'a, 'b> { let repo = if should_update { try!(self.config.shell().status("Updating", - format!("git repository `{}`", self.remote.get_url()))); + format!("git repository `{}`", self.remote.get_location()))); log!(5, "updating git source `{}`", self.remote); try!(self.remote.checkout(&self.db_path)) @@ -135,17 +143,18 @@ impl<'a, 'b> Source for GitSource<'a, 'b> { mod test { use url; use url::Url; + use core::source::Remote; use super::ident; #[test] pub fn test_url_to_path_ident_with_path() { - let ident = ident(&url("https://github.com/carlhuda/cargo")); + let ident = ident(&Remote(url("https://github.com/carlhuda/cargo"))); assert_eq!(ident.as_slice(), "cargo-0eed735c8ffd7c88"); } #[test] pub fn test_url_to_path_ident_without_path() { - let ident = ident(&url("https://github.com")); + let ident = ident(&Remote(url("https://github.com"))); assert_eq!(ident.as_slice(), "_empty-fc065c9b6b16fc00"); } diff --git a/src/cargo/sources/git/utils.rs b/src/cargo/sources/git/utils.rs index 5d560a393..b92fdcb0f 100644 --- a/src/cargo/sources/git/utils.rs +++ b/src/cargo/sources/git/utils.rs @@ -1,5 +1,3 @@ -use url::Url; -use util::{CargoResult, ChainError, ProcessBuilder, process, human}; use std::fmt; use std::fmt::{Show,Formatter}; use std::str; @@ -7,6 +5,9 @@ use std::io::{UserDir,AllPermissions}; use std::io::fs::{mkdir_recursive,rmdir_recursive,chmod}; use serialize::{Encodable,Encoder}; +use core::source::{Location, Local, Remote}; +use util::{CargoResult, ChainError, ProcessBuilder, process, human}; + #[deriving(PartialEq,Clone,Encodable)] pub enum GitReference { Master, @@ -67,18 +68,18 @@ macro_rules! errln( /// GitDatabase. #[deriving(PartialEq,Clone,Show)] pub struct GitRemote { - url: Url, + location: Location, } #[deriving(PartialEq,Clone,Encodable)] struct EncodableGitRemote { - url: String, + location: String, } impl> Encodable for GitRemote { fn encode(&self, s: &mut S) -> Result<(), E> { EncodableGitRemote { - url: self.url.to_str() + location: self.location.to_str() }.encode(s) } } @@ -138,12 +139,12 @@ impl> Encodable for GitCheckout { // Implementations impl GitRemote { - pub fn new(url: &Url) -> GitRemote { - GitRemote { url: url.clone() } + pub fn new(location: &Location) -> GitRemote { + GitRemote { location: location.clone() } } - pub fn get_url<'a>(&'a self) -> &'a Url { - &self.url + pub fn get_location<'a>(&'a self) -> &'a Location { + &self.location } pub fn has_ref(&self, path: &Path, reference: S) -> CargoResult<()> { @@ -180,9 +181,9 @@ impl GitRemote { } fn fetch_location(&self) -> String { - match self.url.scheme.as_slice() { - "file" => self.url.path.clone(), - _ => self.url.to_str() + match self.location { + Local(ref p) => p.display().to_str(), + Remote(ref u) => u.to_str(), } } } diff --git a/src/cargo/util/toml.rs b/src/cargo/util/toml.rs index 8776eac28..e9352a140 100644 --- a/src/cargo/util/toml.rs +++ b/src/cargo/util/toml.rs @@ -2,12 +2,12 @@ use serialize::Decodable; use std::collections::HashMap; use std::str; use toml; -use url::Url; use url; -use core::{SourceId,GitKind}; -use core::manifest::{LibKind,Lib}; -use core::{Summary,Manifest,Target,Dependency,PackageId}; +use core::{SourceId, GitKind}; +use core::manifest::{LibKind, Lib}; +use core::{Summary, Manifest, Target, Dependency, PackageId}; +use core::source::{Location, Local, Remote}; use util::{CargoResult, Require, human}; pub fn to_manifest(contents: &[u8], @@ -95,7 +95,7 @@ pub struct TomlProject { } impl TomlProject { - pub fn to_package_id(&self, namespace: &Url) -> CargoResult { + pub fn to_package_id(&self, namespace: &Location) -> CargoResult { PackageId::new(self.name.as_slice(), self.version.as_slice(), namespace) } } @@ -117,6 +117,15 @@ impl TomlManifest { let mut deps = Vec::new(); + fn to_location(s: &str) -> Location { + if s.starts_with("file:") { + Local(Path::new(s.slice_from(5))) + } else { + // TODO: Don't unwrap here + Remote(url::from_str(s).unwrap()) + } + } + // Collect the deps match self.dependencies { Some(ref dependencies) => { @@ -132,10 +141,9 @@ impl TomlManifest { .unwrap_or_else(|| "master".to_str()); let new_source_id = details.git.as_ref().map(|git| { - // TODO: Don't unwrap here let kind = GitKind(reference.clone()); - let url = url::from_str(git.as_slice()).unwrap(); - let source_id = SourceId::new(kind, url); + let loc = to_location(git.as_slice()); + let source_id = SourceId::new(kind, loc); // TODO: Don't do this for path sources.push(source_id.clone()); source_id @@ -162,7 +170,7 @@ impl TomlManifest { let project = try!(project.require(|| human("No `package` or `project` section found."))); Ok((Manifest::new( - &Summary::new(&try!(project.to_package_id(source_id.get_url())), + &Summary::new(&try!(project.to_package_id(source_id.get_location())), deps.as_slice()), targets.as_slice(), &Path::new("target"), -- 2.30.2